-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Treat specifying a function in the bbsection profile without any directive as noop. #167359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-x86 Author: Rahman Lavaee (rlavaee) ChangesFull diff: https://github.com/llvm/llvm-project/pull/167359.diff 5 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h b/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
index 7b1a5f5019589..ee1f28377f7e4 100644
--- a/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
+++ b/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
@@ -68,17 +68,13 @@ class BasicBlockSectionsProfileReader {
BasicBlockSectionsProfileReader() = default;
- // Returns true if basic block sections profile exist for function \p
- // FuncName.
+ // Returns true if function \p FuncName is hot based on the basic block
+ // section profile.
bool isFunctionHot(StringRef FuncName) const;
- // Returns a pair with first element representing whether basic block sections
- // profile exist for the function \p FuncName, and the second element
- // representing the basic block sections profile (cluster info) for this
- // function. If the first element is true and the second element is empty, it
- // means unique basic block sections are desired for all basic blocks of the
- // function.
- std::pair<bool, SmallVector<BBClusterInfo>>
+ // Returns the cluster info for the function \p FuncName. Returns an empty
+ // vector if function has no cluster info.
+ SmallVector<BBClusterInfo>
getClusterInfoForFunction(StringRef FuncName) const;
// Returns the path clonings for the given function.
@@ -190,7 +186,7 @@ class BasicBlockSectionsProfileReaderWrapperPass : public ImmutablePass {
bool isFunctionHot(StringRef FuncName) const;
- std::pair<bool, SmallVector<BBClusterInfo>>
+ SmallVector<BBClusterInfo>
getClusterInfoForFunction(StringRef FuncName) const;
SmallVector<SmallVector<unsigned>>
diff --git a/llvm/lib/CodeGen/BasicBlockSections.cpp b/llvm/lib/CodeGen/BasicBlockSections.cpp
index e317e1c06741f..52e2909bec072 100644
--- a/llvm/lib/CodeGen/BasicBlockSections.cpp
+++ b/llvm/lib/CodeGen/BasicBlockSections.cpp
@@ -183,8 +183,7 @@ updateBranches(MachineFunction &MF,
// clusters are ordered in increasing order of their IDs, with the "Exception"
// and "Cold" succeeding all other clusters.
// FuncClusterInfo represents the cluster information for basic blocks. It
-// maps from BBID of basic blocks to their cluster information. If this is
-// empty, it means unique sections for all basic blocks in the function.
+// maps from BBID of basic blocks to their cluster information.
static void
assignSections(MachineFunction &MF,
const DenseMap<UniqueBBID, BBClusterInfo> &FuncClusterInfo) {
@@ -197,10 +196,8 @@ assignSections(MachineFunction &MF,
for (auto &MBB : MF) {
// With the 'all' option, every basic block is placed in a unique section.
// With the 'list' option, every basic block is placed in a section
- // associated with its cluster, unless we want individual unique sections
- // for every basic block in this function (if FuncClusterInfo is empty).
- if (MF.getTarget().getBBSectionsType() == llvm::BasicBlockSection::All ||
- FuncClusterInfo.empty()) {
+ // associated with its cluster.
+ if (MF.getTarget().getBBSectionsType() == llvm::BasicBlockSection::All) {
// If unique sections are desired for all basic blocks of the function, we
// set every basic block's section ID equal to its original position in
// the layout (which is equal to its number). This ensures that basic
@@ -308,22 +305,22 @@ bool BasicBlockSections::handleBBSections(MachineFunction &MF) {
if (BBSectionsType == BasicBlockSection::List &&
hasInstrProfHashMismatch(MF))
return false;
- // Renumber blocks before sorting them. This is useful for accessing the
- // original layout positions and finding the original fallthroughs.
- MF.RenumberBlocks();
DenseMap<UniqueBBID, BBClusterInfo> FuncClusterInfo;
if (BBSectionsType == BasicBlockSection::List) {
- auto [HasProfile, ClusterInfo] =
- getAnalysis<BasicBlockSectionsProfileReaderWrapperPass>()
- .getClusterInfoForFunction(MF.getName());
- if (!HasProfile)
+ auto ClusterInfo = getAnalysis<BasicBlockSectionsProfileReaderWrapperPass>()
+ .getClusterInfoForFunction(MF.getName());
+ if (ClusterInfo.empty())
return false;
for (auto &BBClusterInfo : ClusterInfo) {
FuncClusterInfo.try_emplace(BBClusterInfo.BBID, BBClusterInfo);
}
}
+ // Renumber blocks before sorting them. This is useful for accessing the
+ // original layout positions and finding the original fallthroughs.
+ MF.RenumberBlocks();
+
MF.setBBSectionsType(BBSectionsType);
assignSections(MF, FuncClusterInfo);
diff --git a/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp b/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
index 485b44ae4c4aa..c234c0f1b0b34 100644
--- a/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
+++ b/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
@@ -58,22 +58,24 @@ BasicBlockSectionsProfileReader::parseUniqueBBID(StringRef S) const {
}
bool BasicBlockSectionsProfileReader::isFunctionHot(StringRef FuncName) const {
- return getClusterInfoForFunction(FuncName).first;
+ return !getClusterInfoForFunction(FuncName).empty();
}
-std::pair<bool, SmallVector<BBClusterInfo>>
+SmallVector<BBClusterInfo>
BasicBlockSectionsProfileReader::getClusterInfoForFunction(
StringRef FuncName) const {
auto R = ProgramPathAndClusterInfo.find(getAliasName(FuncName));
- return R != ProgramPathAndClusterInfo.end()
- ? std::pair(true, R->second.ClusterInfo)
- : std::pair(false, SmallVector<BBClusterInfo>());
+ return R != ProgramPathAndClusterInfo.end() ? R->second.ClusterInfo
+ : SmallVector<BBClusterInfo>();
}
SmallVector<SmallVector<unsigned>>
BasicBlockSectionsProfileReader::getClonePathsForFunction(
StringRef FuncName) const {
- return ProgramPathAndClusterInfo.lookup(getAliasName(FuncName)).ClonePaths;
+ auto R = ProgramPathAndClusterInfo.find(getAliasName(FuncName));
+ return R != ProgramPathAndClusterInfo.end()
+ ? R->second.ClonePaths
+ : SmallVector<SmallVector<unsigned>>();
}
uint64_t BasicBlockSectionsProfileReader::getEdgeCount(
@@ -494,7 +496,7 @@ bool BasicBlockSectionsProfileReaderWrapperPass::isFunctionHot(
return BBSPR.isFunctionHot(FuncName);
}
-std::pair<bool, SmallVector<BBClusterInfo>>
+SmallVector<BBClusterInfo>
BasicBlockSectionsProfileReaderWrapperPass::getClusterInfoForFunction(
StringRef FuncName) const {
return BBSPR.getClusterInfoForFunction(FuncName);
diff --git a/llvm/test/CodeGen/X86/basic-block-sections-list.ll b/llvm/test/CodeGen/X86/basic-block-sections-list.ll
index 45ef452f4f5c1..d652a540f3e9c 100644
--- a/llvm/test/CodeGen/X86/basic-block-sections-list.ll
+++ b/llvm/test/CodeGen/X86/basic-block-sections-list.ll
@@ -1,17 +1,13 @@
-;; Check the basic block sections list option.
-;; version 0 profile:
-; RUN: echo '!_Z3foob' > %t1
+;; Check that specifying the function in the basic block sections profile
+;; without any other directives is a noop.
;;
-;; version 1 profile:
-; RUN: echo 'v1' > %t2
-; RUN: echo 'f _Z3foob' >> %t2
+;; Specify the bb sections profile:
+; RUN: echo 'v1' > %t
+; RUN: echo 'f _Z3foob' >> %t
;;
-; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t1 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t1 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-NO-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t1 -unique-basic-block-section-names --bbsections-guided-section-prefix=false | FileCheck %s -check-prefix=LINUX-SECTIONS-NO-GUIDED-PREFIX
-; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t2 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t2 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-NO-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t2 -unique-basic-block-section-names --bbsections-guided-section-prefix=false | FileCheck %s -check-prefix=LINUX-SECTIONS-NO-GUIDED-PREFIX
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t > %bbsections
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections > %orig
+; RUN: diff -u %orig %bbsections
define i32 @_Z3foob(i1 zeroext %0) nounwind {
%2 = alloca i32, align 4
@@ -41,45 +37,3 @@ define i32 @_Z3foob(i1 zeroext %0) nounwind {
declare i32 @_Z3barv() #1
declare i32 @_Z3bazv() #1
-
-define i32 @_Z3zipb(i1 zeroext %0) nounwind {
- %2 = alloca i32, align 4
- %3 = alloca i8, align 1
- %4 = zext i1 %0 to i8
- store i8 %4, ptr %3, align 1
- %5 = load i8, ptr %3, align 1
- %6 = trunc i8 %5 to i1
- %7 = zext i1 %6 to i32
- %8 = icmp sgt i32 %7, 0
- br i1 %8, label %9, label %11
-
-9: ; preds = %1
- %10 = call i32 @_Z3barv()
- store i32 %10, ptr %2, align 4
- br label %13
-
-11: ; preds = %1
- %12 = call i32 @_Z3bazv()
- store i32 %12, ptr %2, align 4
- br label %13
-
-13: ; preds = %11, %9
- %14 = load i32, ptr %2, align 4
- ret i32 %14
-}
-
-; LINUX-SECTIONS-NO-GUIDED-PREFIX: .section .text._Z3foob,"ax",@progbits
-; LINUX-SECTIONS: .section .text.hot._Z3foob,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob:
-; LINUX-SECTIONS: .section .text.hot._Z3foob._Z3foob.__part.1,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob.__part.1:
-; LINUX-SECTIONS: .section .text.hot._Z3foob._Z3foob.__part.2,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob.__part.2:
-; LINUX-SECTIONS: .section .text.hot._Z3foob._Z3foob.__part.3,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob.__part.3:
-
-; LINUX-SECTIONS-FUNCTION-SECTION: .section .text._Z3zipb,"ax",@progbits
-; LINUX-SECTIONS-NO-FUNCTION-SECTION-NOT: .section .text{{.*}}._Z3zipb,"ax",@progbits
-; LINUX-SECTIONS: _Z3zipb:
-; LINUX-SECTIONS-NOT: .section .text{{.*}}._Z3zipb.__part.{{[0-9]+}},"ax",@progbits
-; LINUX-SECTIONS-NOT: _Z3zipb.__part.{{[0-9]+}}:
diff --git a/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll b/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll
index d481b147662dc..6e0db20ca0492 100644
--- a/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll
+++ b/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll
@@ -1,6 +1,8 @@
-; RUN: echo "!foo" > %t.order.txt
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt | FileCheck --check-prefix=SOURCE-DRIFT %s
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt -bbsections-detect-source-drift=false | FileCheck --check-prefix=HASH-CHECK-DISABLED %s
+; RUN: echo "v1" > %t
+; RUN: echo "f foo" >> %t
+; RUN: echo "c 0" >> %t
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t | FileCheck --check-prefix=SOURCE-DRIFT %s
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t -bbsections-detect-source-drift=false | FileCheck --check-prefix=HASH-CHECK-DISABLED %s
define dso_local i32 @foo(i1 zeroext %0, i1 zeroext %1) !annotation !1 {
br i1 %0, label %5, label %3
|
3766d26 to
564c38e
Compare
tmsri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
||
| // Renumber blocks before sorting them. This is useful for accessing the | ||
| // original layout positions and finding the original fallthroughs. | ||
| MF.RenumberBlocks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an unrelated change. Should we make this a separate one?
|
It seems LLDB buildbots lldb-remote-linux-ubuntu and lldb-remote-linux-win are broken after this patch. |
Thanks for reporting. The fix is here: #167423 |
Nop. |
It looks like the test is asserting something more specific. How can I run this test? Running |
And it'll tell you why they were unsupported. I'm looking at the failure on AArch64 Linux right now. |
|
Before: After: The test is expecting to see |
After #167359 / 95db31e. A fix was attempted in #167423 but was not quite enough. From what I could understand, in v1 format you have to specify all the basic blocks. Where before !call_me implied they were all cold (I think, very shaky understanding here). For this test we want to see blocks like call_me/foo/call_me. So adding a line for block 1 fixes the tests. It could produce more blocks at some point but I think as long as foo is within two of them, it'll be fine.
|
I've pushed a fix. Given that there is no way to opt back into "v0" behaviour that I can find, and this changes behaviour, this should probably have a release note. Unless this option is too niche to have it, but please justify your choice to do so or not. |
Thanks for fixing this. I did suspect that there would be test use cases for this option, given the ease of use (you don't need to specify each basic block). In this case, we can't use |
No description provided.